-
-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[INFRA-2294][INFRA-2000] Enable Warnings Next Generation Plugin #121
Conversation
Removed FindBugs and CheckStyle maven configuration. Removed old FindBugs and CheckStyle parameters for old plugins. Enabled warnings-ng reporters for Maven, Java and JavaDoc. Enabled warnings-ng reporters for CheckStyle, PMD, and FindBugs. Enabled warnings-ng reporters for Open Tasks.
Build failed; the context from the latest run is: Expand to view
|
Build failed; the context from the latest run is: Expand to view
|
spotbugs is run as part of However it will fail the build and we expect an unstable build for testish (of which spotbugs is one) failures, so I think you still need to tell the build not to fail for spotbugs issues. checkstyle is not as actually getting a consensus on any style is problematic :)
There are at least some plugins that do. Actually this is an issue I had recently with wanrings ng - the quality gate is acrross all tools not just specific ones that it used to be (which allows marking unstable for any spotbugs issue, but just reporting TODOs checkstyle etc). https://github.com/search?p=2&q=org%3Ajenkinsci+findbugs+filename%3AJenkinsfile&type=Code shows quite a few (on page 2 onwards) |
@jtnord you can set a quality gates for a subset of tools using two |
Yes, indeed, you can specify a quality gate for each step, this was one of the main requirements for the warnings-ng redesign. |
Does that mean it is ok to remove the |
What still sounds for me as pipeline newbie kind of strange is that we need to remodel the parameters of my steps as parameters for the So in pseudo-code it would be nice to have: buildPlugin(
post: () -> {
recordIssues tools: [java(), javaDoc()], aggregatingResults: 'true', id: 'java', name: 'Java'
publishCoverage adapters: [jacocoAdapter('target/site/jacoco/jacoco.xml')]
...
}
) |
but if you want an aggregated report... maybe we where doing something else wrong. I will try and reproduce and ask on the users list
you can do that but it's the old way (not sure of all the reasons) but it can mean a plugin upgrade change could require reconfig of many jobs rather than a central place. |
yes. the failOnError should be the spotbugs flag not findings as that is about to be removed (or use both) |
Build failed; the context from the latest run is: Expand to view
|
Build failed; the context from the latest run is: Expand to view
|
Build failed; the context from the latest run is: Expand to view
|
The buildPluginWithGradle step should also be adapted. |
Build failed; the context from the latest run is: Expand to view
|
Build failed; the context from the latest run is: Expand to view
|
Build failed; the context from the latest run is: Expand to view
|
# Conflicts: # vars/buildPlugin.groovy
Does someone know how to fix the unit test failure
Do I need to register my steps somewhere? |
The step works perfectly in our CI build |
@uhafner regarding
Cheers |
|
Spotbugs reporting is being enabled by default in jenkins-infra/pipeline-library#121, update to parent pom 4.x to use spotbugs instead of findbugs.
@oleg-nenashev @slide @uhafner I've opened PRs to every single plugin that uses the findbugs or checkstyle configurations. are we good to go ahead? |
@timja goes above and beyond again! I'm happy to move forward |
Spotbugs reporting is being enabled by default in jenkins-infra/pipeline-library#121, update to parent pom 4.x to use spotbugs instead of findbugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with going ahead and merging it. I will release it with a breaking change notice
Checkstyle and Spotbugs were added to the standard build configuration in jenkins-infra/pipeline-library#121.
trendChartType: 'TOOLS_ONLY', | ||
referenceJobName: referenceJobName | ||
recordIssues enabledForFailure: true, | ||
tools: [java(), javaDoc()], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the addition of Checks, we are suddenly getting tons and tons of warnings. For example, https://github.com/jenkinsci/ldap-plugin/pull/48/files is practically illegible because of irrelevant, low-priority warnings like
NORMAL:
no @param for rootDN
Worse, checks-api
apparently submits annotations even for lines of code which are not modified by a PR and GH displays every annotation, so the Files tab is lost in annotations which are not even related to the diff.
Can we at least turn off display of low-priority Javadoc warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, do not seem to be getting these Checks results in PRs on other plugins. Not sure what would be different here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could work around this by doing one of:
- fixing the javadoc issues
- adjusting the maven javadoc plugin not to complain about these
- extending the javadoc / java parts of buildPlugin to allow overriding the configuration like you can do for spotbugs, checkstyle, pmd already and then do something like
buildPlugin(javadoc: skipPublishingChecks: true
) in your plugin - turning checks off by default for javadoc
thoughts?
cc @XiongKezhi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adjusting the maven javadoc plugin not to complain about these
Could be done in plugin-pom
I guess. I care about genuine problems—@link
target missing, mistyped tag, etc.—but not just silliness like missing @param
which is true for most methods in the Jenkins source base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those warnings are also reported by the warnings-ng plugin: https://ci.jenkins.io/job/Plugins/job/ldap-plugin/job/PR-48/3/, they are just scanned from Java or JavaDoc log files, we can't really do much about the scan and collect process. But you can control the minimal severity of issues to report:
recordIssues tool: java(pattern: '*.log'), minimumSeverity: 'HIGH'
But this is the pipeline grammar of warnings-ng, I'm not sure how to override the configurations in buildPlugin
, maybe buildPlugin(java: minimumSeverity: 'LOW')
? @timja
Also, do you think we should provide another configuration for minimal severity beside the warnings-ng one to only control the published checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think so, just need to configure warnings ng / the Pom appropriately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #154
Checkstyle and Spotbugs were added to the standard build configuration in jenkins-infra/pipeline-library#121.
Spotbugs reporting is being enabled by default in jenkins-infra/pipeline-library#121, update to parent pom 4.x to use spotbugs instead of findbugs.
Spotbugs reporting is being enabled by default in jenkins-infra/pipeline-library#121, update to parent pom 4.x to use spotbugs instead of findbugs.
This PR will remove the old static analysis configuration and replace it with a configuration of the
Warnings Next Generation plugin. See example output to get an impression of the new configuration.
I removed the old configuration of Checkstyle and Findbugs and replaced it with the corresponding Warnings plugin configuration. There are two open things that need to be discussed (and may be added in another PR):
buildPlugin
step. In my jobs I configure all analysis tools in the pom so thatmaven verify
will invoke automatically the static analysis tools that I want (with my configuration files and options). The warnings plugin is smart enough to not break the build if there are no reports written.Detailed changes summary: